Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added implementation for hexagonal segmentation and also staggered square segmentations #1161

Merged
merged 40 commits into from
Sep 20, 2023

Conversation

sebouh137
Copy link
Contributor

@sebouh137 sebouh137 commented Aug 24, 2023

Implemented segmentation variations with hexagonal and square cells, with and without staggering. See arXiv:2308.06939

BEGINRELEASENOTES

  • DDSegmentation: added segmentation HexGrid for hexagonal segmentation
  • DDSegmentation: added CartesionGridXYStaggered for rectangular segmentation that can be staggered for every other layer

ENDRELEASENOTES

@andresailer
Copy link
Member

andresailer commented Aug 25, 2023

Hi @sebouh137 , many thanks for your contribution!

I am wondering if we could merge the SquareGrid with the existing CartesionGrid segmentation implementation?

Also it should be possible to use the "MultiSegmentation" and different offsets to achieve the same result as with the staggered SquareGrid?

Finally some example XML snippets would be good, a test that uses the hexgrid segmentation even better.

Thanks again!

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

Test Results

       6 files         6 suites   7h 33m 9s ⏱️
   355 tests    355 ✔️ 0 💤 0
1 055 runs  1 055 ✔️ 0 💤 0

Results for commit 87689b9.

♻️ This comment has been updated with latest results.

DDCore/src/segmentations/HexGrid.cpp Show resolved Hide resolved
DDCore/src/segmentations/HexGrid.cpp Outdated Show resolved Hide resolved
DDCore/src/segmentations/HexGrid.cpp Show resolved Hide resolved
DDCore/src/segmentations/HexGrid.cpp Outdated Show resolved Hide resolved
DDCore/src/segmentations/HexGrid.cpp Outdated Show resolved Hide resolved
DDCore/src/segmentations/SquareGrid.cpp Outdated Show resolved Hide resolved
DDCore/src/segmentations/SquareGrid.cpp Outdated Show resolved Hide resolved
sebouh137 and others added 7 commits August 25, 2023 15:17
@andresailer
Copy link
Member

Hi @sebouh137 Any thoughts on what I proposed before #1161 (comment) ?

@sebouh137
Copy link
Contributor Author

Hi @andresailer.

Sorry for the delay. Here are my thoughts about your comments. I'll get around to implementing these changes either by the end of this week or early next week.

"I am wondering if we could merge the SquareGrid with the existing CartesionGrid segmentation implementation?"

I agree that would be much simpler. When I was writing this code and testing it for a paper, I could have modified the CartesianGrid segmentation code, but it seemed easier at the time to write a new class as a plugin rather than re-compile the whole dd4hep code (especially since dd4hep was pre-compiled inside a container). But yes it would be possible to replace the SquareGrid with an implementation of a staggering option in CartesianGrid, separately for x, y and z as appropriate.

"Also it should be possible to use the "MultiSegmentation" and different offsets to achieve the same result as with the staggered SquareGrid?"
I tried setting the offset fields in CartesianGrid to be a formula like offset_x="side_length*(layer%2)" in the xml file, but that caused work. Since I couldn't find any documentation on how to do that in the xml file, I ended up writing a segmentation class of my own. I think adding staggering options for CartesianGrid would make more sense

"Finally some example XML snippets would be good, a test that uses the hexgrid segmentation event better."
That is a good idea.

Thanks again!

@sebouh137
Copy link
Contributor Author

Dear @andresailer,
I found where the problem with the check workflows failed this morning. The test-suite must not have defined a field called "layer". So I modified my code so that if staggering is not enabled, it will not attempt to find the value of "layer" (or whatever stagger keyword is provided). Please rerun the check workflows again now that I've fixed this.
Thanks, Sebouh

@sebouh137
Copy link
Contributor Author

Dear @andresailer ,
I am not sure what exactly is going on here, and I would like more details of what it's checking so that I can narrow down whatever I need to change in order to make it pass. All of the tests pass, except for the four below. What is it taking the checksum of, and what reference is it comparing them to?
Thanks,
Sebouh

The following tests FAILED:
22 - t_CLICSiD_check_checksum_EcalBarrel (Failed)
23 - t_CLICSiD_check_checksum_full (Failed)
175 - t_MiniTel_check_checksum_Minitel3 (Failed)
176 - t_MiniTel_check_checksum_full (Failed)
Errors while running CTest

@andresailer
Copy link
Member

Hi @sebouh137,

The checksum is a validity check for detector models to ensure that a detector model still looks exactly the same when changes are made in DD4hep or how DD4hep is compiled, for example when enabling GEANT4_UNITS. This checksum includes the segmentations, so with the modifications this is somehow changed.

Is is probably OK to just update the expected checksums, but maybe @MarkusFrankATcernch can comment on this?

@wdconinc
Copy link
Contributor

The segmentation description in the checksum enumerates all parameters,

for ( const auto& v : p ) {
.

That is likely the cause for the change in checksum since new parameters were added to CartesianGridXY.

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Sep 15, 2023

So you changed the segmentation and consequently the checksum changed. No ?

This is in principle the expected behavior.
It is another question what should actually part of the checksum.
I normally believe that the segmentations should be part of them, because they actually are
highly related to the readout geometry (sort of the segmentation approach in Geant4).
This readout geometry is fundamental to a sub-detector and cannot be that easily changed either.
But maybe we want to have this configurable....

In principle the dump shows the various contributions to the checksum:

  +++ 1                                                             +readout  a54220f77daa9eed +iddesc    8a34357f3cc91ee3 +segment  85d8e05343f2c736
  +++ 1    Minitel2                             de 26ddb5c697d5a3f2 +place    b1216132567d8f10 +daughters cbf29ce484222325 +children 52cc8ff6b960a770
  +++ 1                                                             +readout  8b859f9d89006082 +iddesc    53089002050c5a70 +segment  4446d2a5bd91f515
  +++ 1    Minitel3                             de a381e53d77452f9a +place    8461628938ce25d0 +daughters cbf29ce484222325 +children 30b6d41e344e570d
  +++ 1                                                             +readout  3edc69afe0b565c5 +iddesc    a0a7efb65254fea1 +segment  40ff023e9838d7b2
  +++ 0    world                                de b49f13e86137f0ac +place    18ff43e2fb898b14 +daughters cbf29ce484222325 +children bf1b5030a49d8dac

Are your changes only affecting the readout, idddesc and segment part of the detector elements?

@sebouh137
Copy link
Contributor Author

Dear @MarkusFrankATcernch @wdconinc and @andresailer ,
This is correct that I have added new parameters to CartesianGridXY. The new parameters are options to enable "staggering" (ie, having a cyclical layer-dependent offset in x and y). By default, staggering would is disabled, in order to avoid messing up existing geometries. Further, these parameters are set as optional, so that no errors would occur from having them missing in such existing geometries.
Thanks and best regards,
Sebouh Paul.

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Sep 15, 2023

@sebouh137 @andresailer @wdconinc
From all I can see, there is no way to have a backwards compatible solution to this problem.
If we require backwards compatibility for the checksums, then the current changes to the XY segmentations
must move to the a hexagonal segmentation implementation.
The call

      /// Access to all parameters
      virtual Parameters parameters() const;

in DDSegmentation::Segmentation.h returns all parameters - not just the relevant subset...
there are only two possibilities:

  • Segmentations are not part of the checksum (probably also not readouts and IDDescriptors, as they are related. (not backwards compatible)
  • Hexagonal segmentations may inherit from CartesianGridXY, but are a seperate implementation. This way existing CartesianGridXY segmentations stay intact. This could probably be made backwards compatible.

Both is not really satisfactory, but I see no other way.

@sebouh137
Copy link
Contributor Author

Dear @MarkusFrankATcernch ,
In that case, I will leave HexGrid as I have implemented it. I will remove changes from CartesianGridXY, and create a separate class "CartesianGridXYStaggered", which would have all of the staggering. This should allow everything to be backwards compatible.

@MarkusFrankATcernch
Copy link
Contributor

@sebouh137
I know that code duplication is uncool, but I believe this is unfortunately the only way.

@sebouh137
Copy link
Contributor Author

Dear @MarkusFrankATcernch ,
In that case, I will leave HexGrid as I have implemented it. I will remove changes from CartesianGridXY, and create a separate class "CartesianGridXYStaggered", which would have all of the staggering. This should allow everything to be backwards compatible.

I have now implemented this change. You can run the checks again now.
Best regards,
Sebouh

andresailer and others added 2 commits September 19, 2023 12:37
added "CartesianGridXYStaggered::" to the cellDimensions method in CartesianGridXYStaggered.cpp.
@sebouh137
Copy link
Contributor Author

Dear @andresailer,
I see where this is failing to build. While editing my files, I had forgotten to add "CartesianGridXYStaggered::" to the cellDimensions method in CartesianGridXYStaggered.cpp. I think it should work now.
Best regards,
Sebouh

@andresailer
Copy link
Member

Dear @andresailer, I see where this is failing to build. While editing my files, I had forgotten to add "CartesianGridXYStaggered::" to the cellDimensions method in CartesianGridXYStaggered.cpp. I think it should work now. Best regards, Sebouh

Actually I forgot the CartesianGridXYStaggered:: in dccd89f , but the function declaration was missing before 😄

Thanks for adding the missing piece!

@sebouh137
Copy link
Contributor Author

Dear @andresailer ,
All of the changes you have requested have been marked as resolved, but it is still showing "one change requested". I can't see any non-resolved change on here. Further, all of the checks have now passed. Let me know if there's anything else that you need on my end.
Thanks,
Sebouh

image

@andresailer andresailer merged commit 03a54fd into AIDASoft:master Sep 20, 2023
14 checks passed
@andresailer
Copy link
Member

Thanks again for your contribution!

wdconinc added a commit to eic/eic-spack that referenced this pull request Sep 20, 2023
### Briefly, what does this PR introduce?
This backports support for AIDASoft/DD4hep#1161.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants